Add integ tests for OS metrics(cpu, page fault) #252
Conversation
| public void checkCPUUtilization() throws Exception { | ||
| //read metric from local node | ||
| List<JsonResponseNode> responseNodeList = | ||
| readMetric(PERFORMANCE_ANALYZER_BASE_ENDPOINT + "/metrics/?metrics=CPU_Utilization&agg=sum"); |
There was a problem hiding this comment.
nit: can we create path as constant variable?
There was a problem hiding this comment.
this endpoint is only referenced once here. and I have defined a const var for the common path that can be used by other endpoint. Do we still need to define a seperate static var here ?
| Assert.assertEquals(OSMetrics.CPU_UTILIZATION.toString(), responseData.getField(0).getName()); | ||
| Assert.assertEquals(Constants.DOUBLE, responseData.getField(0).getType()); | ||
| Assert.assertEquals(1, responseData.getRecordSize()); | ||
| Assert.assertTrue(responseData.getRecordAsDouble(0, OSMetrics.CPU_UTILIZATION.toString()) > 0); |
There was a problem hiding this comment.
is there any upper bound we need to check for CPU usage as well other than greater than 0?
There was a problem hiding this comment.
good catch. yes, I think we can check upper bound here for cpu usage. Will add it
| Assert.assertEquals(OSMetrics.PAGING_MAJ_FLT_RATE.toString(), responseData.getField(0).getName()); | ||
| Assert.assertEquals(Constants.DOUBLE, responseData.getField(0).getType()); | ||
| Assert.assertEquals(1, responseData.getRecordSize()); | ||
| Assert.assertTrue(responseData.getRecordAsDouble(0, OSMetrics.PAGING_MAJ_FLT_RATE.toString()) >= 0); |
There was a problem hiding this comment.
won't the value of PAGING_MAJ_FLT_RATE always be >=0?
There was a problem hiding this comment.
yes, it will always be >= 0 if the page metrics are read correctly. unfortunately this is the only sanity check we can do for this metric because the occurance of page fault is out of our control and it would be hard to write a IT to force OS to trigger page fault
| Assert.assertEquals(OSMetrics.PAGING_MIN_FLT_RATE.toString(), responseData.getField(0).getName()); | ||
| Assert.assertEquals(Constants.DOUBLE, responseData.getField(0).getType()); | ||
| Assert.assertEquals(1, responseData.getRecordSize()); | ||
| Assert.assertTrue(responseData.getRecordAsDouble(0, OSMetrics.PAGING_MIN_FLT_RATE.toString()) >= 0); |
There was a problem hiding this comment.
won't the value of PAGING_MIN_FLT_RATE always be >=0?
| return records[index][i]; | ||
| } | ||
| } | ||
| throw new IllegalArgumentException(); |
There was a problem hiding this comment.
in which scenario this IllegalArgumentException will be thrown?
There was a problem hiding this comment.
this exception will be thrown if the record retrieved from PA does not have the "field" we look for
| import com.amazon.opendistro.elasticsearch.performanceanalyzer.integ_test.json.JsonResponseData; | ||
| import com.google.gson.annotations.SerializedName; | ||
|
|
||
| public class JsonResponseNode { |
There was a problem hiding this comment.
nit: Can we add javadoc describing how the various JsonResponse* classes are nested?
There was a problem hiding this comment.
done. added comments on top of each JsonResponse* class
Codecov Report
@@ Coverage Diff @@
## master #252 +/- ##
=============================================
+ Coverage 9.96% 81.12% +71.16%
- Complexity 50 319 +269
=============================================
Files 39 39
Lines 2018 1770 -248
Branches 150 134 -16
=============================================
+ Hits 201 1436 +1235
+ Misses 1809 232 -1577
- Partials 8 102 +94 Continue to review full report at Codecov.
|
commit d141dc1 Author: Ruizhen Guo <[email protected]> Date: Tue Jan 12 10:36:50 2021 -0800 Add integ tests for OS metrics(cpu, page fault) (#252) * Add integ tests for OS metrics(cpu, page fault) commit 8c04a7e Author: Yu Sun <[email protected]> Date: Fri Jan 8 10:14:30 2021 -0800 Improve Test Coverage to 81% (#258) * pending tests to be tested on linuxOS only * add UT for MasterServiceEventMetricsTests * add UT for PerformanceAnalyzerSearchListenerTests * move linuxOS check to @BeforeClass * add UT for MasterServiceMetricsTests * add UT for NodeStatsSettingHandlerTests * add UT for WhoAmI package * remove unused PA reader tests * clean metricQueue before running every test * pending tests to be tested on linuxOS only * add UT for MasterServiceEventMetricsTests * add UT for PerformanceAnalyzerSearchListenerTests * move linuxOS check to @BeforeClass * add UT for MasterServiceMetricsTests * add UT for NodeStatsSettingHandlerTests * add UT for WhoAmI package * remove unused PA reader tests * clean metricQueue before running every test * add UT for PerformanceAnalyzerActionFilterTests * add UT for PerformanceAnalyzerActionListenerTests * add UT for PerformanceAnalyzerControllerTests * add UT for EventLogFileHandlerTests * exclude test for ClusterSettingsManager.java * code review changes Co-authored-by: Yu Sun <[email protected]>
Fixes #, if available:
Description of changes:
Add integ tests for OS metrics(cpu, page fault)
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.